Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update evm_consumer functions implemented #2

Open
wants to merge 29 commits into
base: base/consumer-chain-support
Choose a base branch
from

Conversation

XiangEnze
Copy link

@XiangEnze XiangEnze commented May 19, 2024

Summary

Updated EVMConfig for necessary addresses for evm_consumer, add some functions to avoid duplicates or large functions.

Import op-bindings:

go get "github.com/ethereum-optimism/optimism/op-bindings/bindings"

so we can interact with the L1 contract like the L2OutputOracle

Replaced evmclient with l1Client and l2Client, so we can reuse both to get information L1 and L2.
Using ethclient instead of rpc.client made functions much more lightweight.
Deleted useless functions.
Note that loading full blocks by BlockByNumber requires two requests, so use HeaderByNumber if we don't need all transactions or uncle headers.


Updated

QueryLatestFinalizedBlocks()
QueryBlocks()
QueryBlock()
QueryIsBlockFinalized()
queryLatestFinalizedNumber()
  • sdk "github.com/cosmos/cosmos-sdk/types" not used any more, already deleted
  • queryLatestBlocks() is not used any more, already deleted
  • implemented QueryLatestFinalizedBlocks()

Next Steps:

  • implement QueryActivatedHeight

Test Plan

make mock-gen
make test

Copy link
Member

@bap2pecs bap2pecs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XiangEnze XiangEnze changed the base branch from feat/SNA-425-QueryBestBlock-refactor to feat/SNA-427-QueryBlock-refactor May 20, 2024 16:34
Hash string
}

var Batch []rpc.BatchElem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here Batch should be batch


var Batch []rpc.BatchElem

InitBatch(Batch, startnumber, count, "ascent")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitBatch -> initBatch

if you check other function in the file, first-letter-capitalization is only for interface function.

return nil, fmt.Errorf("error:%s", err)
}

ibatch := &types.BlockInfo{
Copy link
Member

@bap2pecs bap2pecs May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does ibatch mean? it's a block not a batch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to block


for _, batch := range batchArray {
nb := batch.Result.(*Block)
num, err := strconv.ParseUint(strings.TrimPrefix(nb.Number, "0x"), 16, 64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks very wrong. query a block should be very simply and standard operation

the entire implementation of this function should be concise

@bap2pecs
Copy link
Member

discussed w @XiangEnze and @lesterli offline. a few things:

  • rpc.Client is too low-level. we should use ethclient. no idea why we chose to use rpc.Client. it's not referenced anywhere in any docs
  • there is no good way to use ethclient to send batch rpc requests. but we can use standard HTTP library in golang to do it. similar to this. but this will increase the code complexity a lot. for now, we will just do a simple for-loop and worry about optimization later.
  • to get block: https://goethereumbook.org/block-query/
  • to query contract: https://goethereumbook.org/smart-contract-read/

references:


var blocks []*types.BlockInfo

/*will be implemented in next PR, wait for "refactor: QueryLatestFinalizedBlocks(count uint64) -> QueryLatestFinalizedBlock() #347"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok it's already merged now. see the msg in TG group and update accordingly

you also need to sync changes from upstream and change base branch

}

func (ec *EVMConsumerController) queryLatestBlocks(startKey []byte, count uint64, status finalitytypes.QueriedBlockStatus, reverse bool) ([]*types.BlockInfo, error) {
var blocks []*types.BlockInfo

// Can be deleted for never using
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's easy to do, you can just remove it here

@XiangEnze XiangEnze changed the base branch from feat/SNA-427-QueryBlock-refactor to base/consumer-chain-support May 24, 2024 12:57
Makefile Outdated
@@ -56,6 +56,7 @@ build-docker:

.PHONY: test
test:
make mock-gen
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not exist

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted

Comment on lines 13 to 14
RPCL1Addr string `long:"rpc-address" description:"address of the L2 RPC server to connect to"`
RPCL2Addr string `long:"rpc-address" description:"address of the L1 RPC server to connect to"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"RPCL1Addr - address of the L2 RPC"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

RPCAddr string `long:"rpc-address" description:"address of the rpc server to connect to"`
RPCL1Addr string `long:"rpc-address" description:"address of the L2 RPC server to connect to"`
RPCL2Addr string `long:"rpc-address" description:"address of the L1 RPC server to connect to"`
L2OutputOracleContractAddress string `long:"sol-address" description:"address of the L2output smart contract"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L2output -> L2OutputOracle

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

RPCL1Addr string `long:"rpc-address" description:"address of the L2 RPC server to connect to"`
RPCL2Addr string `long:"rpc-address" description:"address of the L1 RPC server to connect to"`
L2OutputOracleContractAddress string `long:"sol-address" description:"address of the L2output smart contract"`
BitcoinStackingContractAddress string `long:"sol-address" description:"address of the Bitcoinstaking smart contract"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this is also called long:"sol-address"? sounds wrong

what does "long" mean here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants